Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added support for Seek on iterator and IteratorWithRange on storage #85

Merged
merged 4 commits into from
Feb 19, 2018

Conversation

jbpin
Copy link
Contributor

@jbpin jbpin commented Feb 5, 2018

As discuss in issue #82 Here is the pull request.

I write light test on memory.. I'm looking forward for your code review.

@db7 db7 requested review from db7 and SamiHiltunen February 5, 2018 15:24
iterator.go Outdated
@@ -9,6 +9,7 @@ type Iterator interface {
Key() string
Value() (interface{}, error)
Release()
Seek(key []byte) bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goka.Iterator has keys as strings. Does Seek() make sense when based on strings?

@jbpin
Copy link
Contributor Author

jbpin commented Feb 19, 2018

Hi guys, any update on this ?

@db7
Copy link
Collaborator

db7 commented Feb 19, 2018

Actually yes, there was this question above: The key passed to Seek is a byte slice, but all other keys passed to the goka.Iterator are strings. Can the key passed to Seek be a string?

As far as I remember, storage.Iterator uses byte slices too. So there it isn't a problem.

@jbpin
Copy link
Contributor Author

jbpin commented Feb 19, 2018

Sure ;)

iterator.Seek([]byte("this is my key"))

@db7
Copy link
Collaborator

db7 commented Feb 19, 2018

;)

What I meant is that the whole interface of goka.Iterator is defined with keys as strings. So Seek() should take a string, like this:

type Iterator interface {
	Next() bool
	Key() string
	Value() (interface{}, error)
	Release()
	Seek(key string) bool
}

Similarly, creating the iterator would ideally be done with strings:

func (f *file) IteratorWithRange(start, limit string) (Iterator, error) {

Is there any issue with that?

@jbpin
Copy link
Contributor Author

jbpin commented Feb 19, 2018

;) I see... In fact I added like that so that it's complient with the interface of the leveldb library.
https://github.com/syndtr/goleveldb/blob/master/leveldb/iterator/iter.go

@db7
Copy link
Collaborator

db7 commented Feb 19, 2018

I guessed so. The storage.Iterator is defined with keys as byte slices because LevelDB uses byte slices.

Still, since we work with string keys at the goka package level, I think it would make sense to keep string keys there. Or change all keys to byte slices, but that would break way too many things.

Change iterator Seek method to take string and not []byte
@jbpin
Copy link
Contributor Author

jbpin commented Feb 19, 2018

ok get it.. I update the code

@db7
Copy link
Collaborator

db7 commented Feb 19, 2018

Excellent! And this one should take strings too: func (v *View) IteratorWithRange(start, limit []byte) (Iterator, error) {

Besides these []byte thing, the PR looks pretty good to me. Thanks for contributing.

@jbpin
Copy link
Contributor Author

jbpin commented Feb 19, 2018 via email

something like that
Copy link
Collaborator

@db7 db7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@db7 db7 merged commit cea03b6 into lovoo:master Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants